This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

Just FYI to anyone doing a more proper review - the reason a large number of files are changed is because isort sorted the imports.

Edit: Now a lot of fixes recommended from bandit (e.g. using defusedxml, specifying md5 as not being used for security).

So the issue is that the gmail address validation is acting differently across different platforms and python versions.
Specifically, the test of validation behavior for email addresses of ",com" instead of ".com" is passing (mostly on older versions). I'm guessing there is a package we don't have pinned to a specific version and it's exhibiting different behavior based on what version is installed.

I did a little investigating. The test failure is on the gmail connector, which inherits from the sendmail connector; the sendmail connector has a method _validate_email_string which calls validate_email which appears to be this package. Our requirements.txt pins 1.3 which is the most recent version; unfortunately that most recent version was published 9 years ago. I'm not sure why the tests started failing just now (my guess is it's something to do with bumping the version of pytest) but regardless we probably don't want to be using a library that hasn't been updated in 9 years.

There is an actively maintained fork but it says "Pypi removed newer versions due to license issues". So...maybe the thing to do is to migrate to a different email validation package entirely? Josh Tauberer has an email validation package we could switch to.

I did try reverting to the old version of pytest et al and it was still failing, but I agree it's probably a good idea to use better validation. (especially since the old solution allowed ",com" which is not valid afaik).
I should be able to do that someday soon.

I mentioned you in the other thread but I'll also add here that this failure does not seem to be specific to this PR. It's happening in all other recent PRs but not the most recent merged commit (which was 11:56am EDT yesterday Sep 17th). Given that there have been no updates to the github images between then and now, and the errors are happening at least on mac and ubuntu, it seems unlikely to be caused by a change to the runners. Maybe a change to uv? I don't think we pin our version of uv on install and there have been one, possibly two releases of uv since noon yesterday.

Actually in this PR we are using a pinned version of uv:
https://github.com/move-coop/parsons/pull/1132/files#diff-3e744f163053a3e58aeec8dede7e3711d8a87b2249a31fd94a884200b0aec1c7R31-R32

This is so weird. It's like whatever was causing the issue was fixed in Ubuntu for 3.9 but not in Mac/Windows until 3.12.

For clarity, when you say "this now works great on 3.12" - do you mean that a validation error is raised as expected when given ",com"?

I'm trying to wrap my head around where the problem could be coming from. Because until recently these same tests passed (on ubuntu and mac, at least) meaning that a validation error was raised, no? So something changed (either by parsons, uv, github, python, etc) that caused the validation error not to be raised. But now with this new package it's sometimes being raised.

I think that one of our dependencies must have updated or something like that. Like if one of our dependencies has a subdependency that is not version pinned, it could potentially do something like this.

What I did to get the tests passing on 3.12 was change out the email validation library, but it still is passing ,con on certain configurations.

We just recently updated some dependencies, so it could be related to that. But if I remember correctly, that pull request had all of the checks passed.

Unless maybe the checks didn't run because a particular file was being updated...

Probably time to jump into some actions logs and see what changed.

I fixed the email parsing issue with #1146, if someone wants to give it the ol' review :)

@austinweisgrau @bmos alas now all the tests are failing. FYI I did resolve the merge conflict between @austinweisgrau's PR and this one, perhaps incorrectly. But it also may be that the PR doesn't fully address the issue (I'm still unclear how a problem in Python 3.12 could cause our tests for 3.8-3.11 to fail, esp in the weird pattern they did (mostly but not entirely on Windows & Mac rather than Ubuntu)

IMO this PR should be split up a bit. In particular the isort formatting should be its own PR. Because hundreds of files are changed with the isort formatting, it makes it very difficult to detect any other unwanted changes that are happening here, and also difficult to evaluate, for example, what changes in particular may be resulting in test failures.

Yeah that's fair. We'd talked about moving the isort stuff to a separate PR a few weeks ago for precisely that reason but got derailed by the email address bug. @bmos if you'd be up for it can you split the PRs?

I would be happy to, that should be pretty easy.

I'm also pretty sure that none of these changes are creating test failures, but again, totally happy to break that out.

Thanks so much @bmos, regardless of the causes it'll be easier to read & reason about this way.

Okay, that should take care of that.

Looks good - thank you!

hell yea good work